Skip to content

refactor(updater): simplify digest verification flows - #549

Merged
magrhino merged 2 commits into
mainfrom
codex/sonar-digest-verification
Jul 30, 2026
Merged

refactor(updater): simplify digest verification flows#549
magrhino merged 2 commits into
mainfrom
codex/sonar-digest-verification

Conversation

@magrhino

@magrhino magrhino commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

Reduce the cognitive complexity of digest verification without changing updater success/failure decisions, stale pending-digest handling, untrusted verification behavior, or diagnostic output.

  • Evaluate each expected digest through a focused requirement helper and keep processing all requirements after failures.
  • Evaluate each digest-pin update independently, separating target re-resolution, image matching, and failure logging.
  • Preserve stale-target messages, sanitized resolver errors, mismatch details, and unmatched-image errors.
  • Add focused regression coverage proving later requirements and updates are still checked after an earlier failure.
  • Resolved Sonar issues: AZ60TpT-vxKY-zkovz2k, AZ60TpT-vxKY-zkovz2l.
  • Intentionally deferred from this PR: updates.py, init_config.py, and web_scheduler.py findings remain separate module/safety boundaries.
  • Existing issue search found no matching open issue.

Scope

  • This PR has one reviewable objective
  • Follow-up work, stacked PRs, or intentionally deferred changes are called out here: init configuration and scheduler selection remain independent cleanup PRs; updater lock/command and scheduler reservation work remain deferred for dedicated safety-boundary coverage.

Checklist

  • Tests added or updated
  • README.md or docs/ updated if install, deployment, config, or user-facing behavior changed — N/A, behavior and user-facing output are preserved
  • template.env updated if environment variables or example config changed — N/A, no configuration changed
  • CHANGELOG.md updated in ## [Unreleased] section — N/A, maintainability-only change

Test plan

  • tests/run-all.sh — N/A, full shell/container suite is outside this Python updater scope

  • Focused test(s):

    • .venv/bin/python -m pytest -q tests/test_python_update_from_wud_core.py tests/test_python_update_from_wud_digest_pins.py (37 passed before the review follow-up)
    • .venv/bin/python -m unittest tests.test_python_update_from_wud_digest_pins.UpdateFromWudDigestPinTests.test_digest_pin_verification_checks_all_updates_after_digest_failure (passed)
    • tests/test-docker-update-from-wud.sh (passed in the intended non-PTY mode)
    • .venv/bin/ruff check src/wudup/updater_lifecycle_digest.py tests/test_python_update_from_wud_core.py tests/test_python_update_from_wud_digest_pins.py
    • tests/run-all.sh --python (1313 passed, 1 skipped)
    • git diff --check
  • Docker or Compose validation, if container behavior changed: reviewed as Docker-adjacent; no container packaging or Compose behavior changed, the existing Docker E2E check is green, and the full Python suite passed.

Danger: docker-review-complete — Digest verification was reviewed as Docker-adjacent; no container or Compose behavior changed, the focused digest-pin regression passed, the full Python suite passed, and the PR's Docker E2E check is green.

An initial PTY invocation of tests/test-docker-update-from-wud.sh waited at its intentional non-interactive confirmation test and was interrupted; the required non-PTY rerun passed. Sonar local file analysis was attempted for all three changed files, but the MCP analyzer returned “Failed to request analysis of the list of files.”

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@magrhino, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f23c21a-a3a3-4e01-9166-9a12b61cc609

📥 Commits

Reviewing files that changed from the base of the PR and between 652a502 and 9ea252b.

📒 Files selected for processing (1)
  • tests/test_python_update_from_wud_digest_pins.py
📝 Walkthrough

Walkthrough

Expected-digest and digest-pin verification, refactored into helper flows, now continue checking all requirements and updates after failures. Tests confirm aggregate failure results alongside complete verification attempts. So, predictable verification becomes.

Changes

Digest verification

Layer / File(s) Summary
Expected-digest verification flow
src/wudup/updater_lifecycle_digest.py, tests/test_python_update_from_wud_core.py
Helper methods evaluate each expected requirement, handle untrusted results, and verify all requirements after an earlier mismatch.
Digest-pin verification flow
src/wudup/updater_lifecycle_digest.py, tests/test_python_update_from_wud_digest_pins.py
Digest-pin resolution failures and image verification are centralized, while all planned updates continue to be checked.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed Clear and specific; it summarizes the digest-verification refactor well.
Description check ✅ Passed All required sections are present, and the scope, checklist, and test plan are sufficiently filled out.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors digest verification in the updater to split requirement-level and update-level checks into focused helper methods while preserving existing behavior and logging, and adds regression tests to ensure all requirements/updates are processed even after failures.

File-Level Changes

Change Details Files
Refactor expected-digest verification into dedicated helpers that encapsulate image scanning, result handling, and logging while preserving behavior.
  • Replaces inline loop over requirements in _verify_expected_digests with calls to a new _verify_expected_digest_requirement helper and continues processing even when a requirement fails.
  • Introduces _verify_expected_digest_images to centralize matching compose images to expected targets and invoking the digest verifier, returning both digest and stale results plus match state.
  • Keeps untrusted, mismatch, stale-pending, and unmatched-image logging and side effects intact but routed through the new helpers.
src/wudup/updater_lifecycle_digest.py
Refactor digest-pin update verification into smaller helpers that separate target re-resolution, image verification, and logging, while maintaining existing failure and logging semantics.
  • Changes _verify_digest_pin_updates to delegate per-update handling to a new _verify_digest_pin_update helper and to keep iterating over all updates even if some fail.
  • Adds _log_digest_pin_resolution_failure to encapsulate stale-digest and resolution-error logging, including sanitized resolver error messages.
  • Adds _verify_digest_pin_images to encapsulate compose-image matching for digest-pin targets and verification, returning digest result and match state while preserving mismatch and unmatched-image logging.
src/wudup/updater_lifecycle_digest.py
Add regression tests to prove that all expected-digest requirements and digest-pin updates are still checked after an earlier failure.
  • Adds test_expected_digest_verification_checks_all_requirements to verify that _verify_expected_digests invokes the digest verifier for later requirements even when an earlier one fails and ultimately reports failure.
  • Adds test_digest_pin_verification_checks_all_updates_after_failure to validate that _verify_digest_pin_updates still processes subsequent updates when _verify_digest_pin_update_target fails for an earlier update.
tests/test_python_update_from_wud_core.py
tests/test_python_update_from_wud_digest_pins.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@magrhino
magrhino marked this pull request as ready for review July 29, 2026 23:33

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • The new helper methods _verify_expected_digest_images and _verify_digest_pin_images share very similar control flow; consider extracting a generic internal helper (e.g., parametrized by expected image and digest) to reduce duplication and keep the matching/verification logic in one place.
  • Both _verify_expected_digest_images and _verify_digest_pin_images now return raw tuples (digest_result, stale_result/matched); introducing a small internal dataclass or namedtuple for these results would make the calling code more self-documenting and less error-prone than relying on positional tuple unpacking.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new helper methods `_verify_expected_digest_images` and `_verify_digest_pin_images` share very similar control flow; consider extracting a generic internal helper (e.g., parametrized by expected image and digest) to reduce duplication and keep the matching/verification logic in one place.
- Both `_verify_expected_digest_images` and `_verify_digest_pin_images` now return raw tuples (`digest_result, stale_result/matched`); introducing a small internal dataclass or namedtuple for these results would make the calling code more self-documenting and less error-prone than relying on positional tuple unpacking.

## Individual Comments

### Comment 1
<location path="src/wudup/updater_lifecycle_digest.py" line_range="237-246" />
<code_context>
+    def _verify_digest_pin_images(
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicated image-verification loops by consolidating into a single helper

`_verify_digest_pin_images` duplicates most of `_verify_expected_digest_images` (iteration over `images`, `image_matches_resolved_target`, `self.digest_verifier.verify`, and early break on `digest_result.ok`), differing mainly in `allow_repo` and the digest source. Extract a single helper that takes `resolved_image`, `expected_digest`, and `allow_repo` so both paths share the verification logic and future changes only need to be made in one place.

Suggested implementation:

```python
    def _verify_images(
        self,
        *,
        resolved_image: str,
        expected_digest: str,
        images: Sequence[str],
        allow_repo: bool,
    ) -> tuple[DigestCheckResult | None, bool]:
        matched = False
        digest_result: DigestCheckResult | None = None

        for image in images:
            if not image_matches_resolved_target(
                image,
                resolved_image,
                allow_repo=allow_repo,
            ):
                continue

            matched = True
            digest_result = self.digest_verifier.verify(
                image=image,
                resolved_image=resolved_image,
                expected_digest=expected_digest,
            )
            if digest_result.ok:
                break

        return digest_result, matched

    def _verify_digest_pin_images(
        self,
        update: DigestPinUpdate,
        images: Sequence[str],
    ) -> tuple[DigestCheckResult | None, bool]:
        return self._verify_images(
            resolved_image=update.resolved_image,
            expected_digest=update.expected_digest,
            images=images,
            allow_repo=False,

```

I can only see part of the file, so to complete the refactor you should:

1. **Update `_verify_expected_digest_images`** to use the new helper instead of its own loop. For example, replace its internal loop with something along the lines of:
   ```python
   digest_result, matched = self._verify_images(
       resolved_image=update.resolved_image,
       expected_digest=update.expected_digest,
       images=images,
       allow_repo=True,  # or False, depending on the original behavior
   )
   ```
   and return `digest_result, matched` from the function.

2. **Align the helper call signature with the existing verifier usage**:
   - If `self.digest_verifier.verify` currently has a different signature (e.g. positional args instead of named, or additional parameters), update the call inside `_verify_images` to match the existing pattern.
   - If `allow_repo` was previously passed to `image_matches_resolved_target` or used differently in `_verify_expected_digest_images` / `_verify_digest_pin_images`, ensure that logic is preserved when wiring through `_verify_images`.

3. **Ensure the expected digest for digest-pin paths is correctly sourced**:
   - If `DigestPinUpdate` uses a different attribute than `expected_digest` for the digest to verify (e.g. `update.pin_digest` or similar), adapt the `expected_digest=...` argument in `_verify_digest_pin_images` accordingly.

Once those changes are made, both `_verify_expected_digest_images` and `_verify_digest_pin_images` will share the same verification logic via `_verify_images`, eliminating duplicated loops and centralizing future changes in one place.
</issue_to_address>

### Comment 2
<location path="tests/test_python_update_from_wud_core.py" line_range="95-104" />
<code_context>
+    def test_expected_digest_verification_checks_all_requirements(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding coverage for the stale-digest path to ensure `_mark_stale_pending_digest` is still invoked and later requirements are evaluated.

This test already verifies that `_verify_expected_digests` continues evaluating requirements after an early mismatch and still returns `False`. To fully cover the behavior, add a sibling test for the `stale-digest` branch in `_verify_expected_digest_requirement` that:
- Sets `digest_verifier.verify.side_effect` so the first requirement yields a `DigestCheckResult` with `reason="stale-digest"` and `ok=False`.
- Patches `runner.lifecycle._mark_stale_pending_digest` and asserts it is called once for the first requirement.
- Asserts the second requirement is still verified and the overall return value remains `False`.

This will lock in the behavior around stale pending digests described as important in the PR.

Suggested implementation:

```python
            runner.lifecycle._expected_digest_failure_reason(stack, matches),
            STALE_PENDING_DIGEST_REASON,
        )

    def test_expected_digest_verification_marks_stale_pending_digest_and_checks_all_requirements(
        self,
    ) -> None:
        stack_dir = self.make_stack(
            "app",
            [
                ("first", "repo/first:latest", "cid-first"),
                ("second", "repo/second:latest", "cid-second"),
            ],
        )
        stack = ComposeStack(
            index=1,
            directory=stack_dir,
        )

        # Create a runner and two requirements mirroring the existing test setup.
        runner = self._make_runner(stack)  # adjust helper as used in sibling test

        first_requirement = mock.Mock(name="first_requirement")
        second_requirement = mock.Mock(name="second_requirement")
        requirements = [first_requirement, second_requirement]

        stale_result = DigestCheckResult(ok=False, reason="stale-digest")
        second_result = DigestCheckResult(ok=False, reason="unexpected-digest")

        with patch.object(
            runner.lifecycle.digest_verifier,
            "verify",
            side_effect=[stale_result, second_result],
        ) as verify, patch.object(
            runner.lifecycle,
            "_mark_stale_pending_digest",
        ) as mark_stale:
            result = runner.lifecycle._verify_expected_digests(stack, requirements)

        # The stale-digest requirement should cause the pending digest to be marked as stale.
        assert result is False
        assert verify.call_count == 2
        verify.assert_has_calls(
            [
                mock.call(stack, first_requirement),
                mock.call(stack, second_requirement),
            ]
        )
        mark_stale.assert_called_once()
        # Ensure we mark stale for the first requirement; adjust arguments to match implementation.
        first_mark_call = mark_stale.call_args
        assert first_mark_call is not None

    def test_expected_digest_verification_checks_all_requirements(self) -> None:

```

1. This new test assumes `mock` and `patch` are available from `unittest.mock`. If they are not already imported at the top of this file, add:
   - `from unittest import mock`
   - `from unittest.mock import patch`
2. The helper `self._make_runner(stack)` and attribute `runner.lifecycle.digest_verifier` are inferred from the existing test patterns; align this to whatever the sibling test `test_expected_digest_verification_checks_all_requirements` uses to construct the runner and access the `digest_verifier`.
3. The call signatures for `runner.lifecycle._verify_expected_digests` and `_mark_stale_pending_digest` are inferred. If `_mark_stale_pending_digest` takes specific arguments (e.g., `(stack, requirement)` or just `(stack,)`), update the `mark_stale.assert_called_once()` / `assert_has_calls` assertions accordingly to match the real signature.
4. To keep the style consistent, you may want to mirror the exact stack/requirements setup of `test_expected_digest_verification_checks_all_requirements` (e.g., how requirements are constructed) rather than using `mock.Mock` placeholders; adjust `first_requirement` and `second_requirement` creation to match that existing test.
</issue_to_address>

### Comment 3
<location path="tests/test_python_update_from_wud_digest_pins.py" line_range="180-189" />
<code_context>
+    def test_digest_pin_verification_checks_all_updates_after_failure(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test where the first digest-pin update fails during digest verification or image matching, not only during target resolution.

This test currently covers the case where the first update fails at target resolution. With the new split between resolution and image verification, you should also add a test where the first update resolves successfully but fails during image matching or digest verification (e.g. `DigestCheckResult` with `ok=False`, `reason="digest-mismatch"`), and the second update still resolves and verifies. For example:
- Mock `_verify_digest_pin_update_target` to succeed for both updates.
- Set `digest_verifier.verify.side_effect` so the first call fails and the second succeeds.
- Assert `digest_verifier.verify.call_count == 2` and that the overall result is `False`.
This will ensure coverage of both resolution failures and verification/matching failures under the new helper structure.

Suggested implementation:

```python
    def test_digest_pin_verification_checks_all_updates_after_failure(self) -> None:
        stack = ComposeStack(
            index=1,
            directory=self.root,
            file="docker-compose.yml",
            name="app",
            images=("docker.io/repo/second:2.0",),
            service_images=(
                ServiceImage("second", "docker.io/repo/second:2.0"),
            ),
        )

    def test_digest_pin_verification_checks_all_updates_after_verification_failure(
        self,
        docker_manifest_resolver: DockerManifestResolver,
        digest_verifier: DigestVerifier,
    ) -> None:
        """
        Ensure that when the first digest-pin update resolves successfully but fails during
        digest verification / image matching, we still attempt to verify the remaining updates.
        """
        stack = ComposeStack(
            index=1,
            directory=self.root,
            file="docker-compose.yml",
            name="app",
            images=(
                "docker.io/repo/first:1.0",
                "docker.io/repo/second:2.0",
            ),
            service_images=(
                ServiceImage("first", "docker.io/repo/first:1.0"),
                ServiceImage("second", "docker.io/repo/second:2.0"),
            ),
        )

        config = load_config(self.root)

        # Make target resolution succeed for both digest-pin updates.
        # The helper under test is expected to call `_verify_digest_pin_update_target`
        # for each candidate update before performing digest verification.
        def _verify_digest_pin_update_target(update):
            return DigestResolveResult(ok=True, reason=None, resolved_tag=update.target)

        # First digest verification fails, second succeeds.
        digest_verifier.verify.side_effect = (
            DigestCheckResult(ok=False, reason="digest-mismatch"),
            DigestCheckResult(ok=True, reason=None),
        )

        # Run the update logic that performs digest-pin verification.
        # This call is expected to:
        # 1. Resolve both updates via `_verify_digest_pin_update_target`
        # 2. Call `digest_verifier.verify` for each resolved update
        result = update_from_wud_digest_pins(
            config=config,
            stack=stack,
            docker_manifest_resolver=docker_manifest_resolver,
            digest_verifier=digest_verifier,
            verify_digest_pin_update_target=_verify_digest_pin_update_target,
        )

        # Both updates should have been verified even though the first one failed.
        assert digest_verifier.verify.call_count == 2

        # Overall digest-pin verification should fail because one verification failed.
        assert result.ok is False

```

To integrate this test correctly with the rest of the codebase, you will likely need to:
1. Adjust the signature of `test_digest_pin_verification_checks_all_updates_after_verification_failure` to match the existing fixture setup (e.g. if `docker_manifest_resolver`, `digest_verifier`, or `update_from_wud_digest_pins` are provided via fixtures or imported differently).
2. Import `update_from_wud_digest_pins` at the top of the test module (e.g. `from wudup.python_update_from_wud_digest_pins import update_from_wud_digest_pins`) or adapt the call to the correct function that performs digest-pin verification.
3. If `_verify_digest_pin_update_target` is an internal helper in the production code rather than something passed in, replace the injected `verify_digest_pin_update_target` parameter with the appropriate mocking strategy (e.g. monkeypatching the helper in the target module to always return `DigestResolveResult(ok=True, ...)`).
4. Ensure that `DigestCheckResult` and `DigestResolveResult` are used consistently with how they are defined in `wudup.digest_verifier` (field names and constructor usage); adapt the `reason` / `resolved_tag` attributes if their API differs slightly.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/wudup/updater_lifecycle_digest.py
Comment thread tests/test_python_update_from_wud_core.py
Comment thread tests/test_python_update_from_wud_digest_pins.py
@sonarqubecloud

Copy link
Copy Markdown

@magrhino
magrhino merged commit 84d8df6 into main Jul 30, 2026
22 checks passed
@magrhino
magrhino deleted the codex/sonar-digest-verification branch July 30, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant